Fix line breaking for attributes in inheritance clauses by grouping them with their type #1039
Conversation
TTOzzi
requested review from
allevato,
bnbarham,
hamishknight and
rintaro
as code owners
| @@ -2357,7 +2357,9 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { | |||
| } | |||
|
|
|||
| override func visit(_ node: InheritedTypeSyntax) -> SyntaxVisitorContinueKind { | |||
| before(node.firstToken(viewMode: .sourceAccurate), tokens: .open) | |||
There was a problem hiding this comment.
Can you move this new logic into the visit method for AttributedTypeSyntax instead? Then it we'll get the grouping in a bunch of other places that we have attributed/modified types, like @Sendable () -> Void or consuming T, not just in inheritance lists.
There was a problem hiding this comment.
Thank you for the feedback! I've applied the changes.
TTOzzi
force-pushed
the
encapsulating-inherited-type
branch
from
6b77fa9 to
3041452
Compare
| cmp: @escaping (R) -> | ||
| () | ||
| cmp: | ||
| @escaping (R) -> () |
There was a problem hiding this comment.
Nice! I was hoping we'd find some others with that change.
There was a problem hiding this comment.
I believe this one is causing a bunch of differences in eg. sourcekit-lsp:
- requestHandler: @Sendable @escaping (
- RequestType, Workspace, LanguageService
- ) async throws ->
+ requestHandler:
+ @Sendable @escaping (
+ RequestType, Workspace, LanguageService
+ ) async throws ->
(and others like it)
I wouldn't have expected any change there though 🤔? The @Sendable @escaping ( still fits on the same line as requestHandler:. EDIT: Although maybe it does make sense - the type doesn't fit on the same line, it's now fully grouped, and thus put on a new line + indented.
There was a problem hiding this comment.
Yeah, the group around the whole type now forces the line break since the whole type doesn't fit. I personally prefer that behavior (citing The Rectangle Rule), because I think a case like the one in the test diff above is one where the new version is clearly better, even though the one you posted from SourceKit doesn't seem so bad.
The difference between those two might be that in the test, we took something that didn't fit on a single line and made it fit on a single line, so that's (unambiguously, IMO) an improvement. In the SourceKit example, that's not the case, so it's less of a big win.
A future version of the formatter could try to make decisions like that, but it would require backtracking, so I've tried to resist that complexity.
Resolve #1032